Skip to content

Update urc#78

Merged
da-bao-jian merged 3 commits intomainfrom
update-urc
May 8, 2025
Merged

Update urc#78
da-bao-jian merged 3 commits intomainfrom
update-urc

Conversation

@will-luban
Copy link
Copy Markdown
Contributor

@will-luban will-luban commented May 7, 2025

Summary by CodeRabbit

  • Refactor
    • Updated time validation logic to use wall-clock time instead of block height for fraud proof window checks.
    • Centralized and streamlined slashing logic with enhanced protocol-specific execution and event reporting.
  • Tests
    • Adjusted test simulations to use timestamp-based time advancement instead of block number increments for fraud proof scenarios.
  • Chores
    • Exposed minimum withdrawal delay as a public variable for easier access.
    • Serialized deployment addresses for improved tracking.

@will-luban will-luban requested a review from da-bao-jian May 7, 2025 05:24
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented May 7, 2025

Walkthrough

The changes update time-based logic in both contract and test code, switching fraud proof window checks from block number to block timestamp. The LinglongSlasher contract was refactored to centralize slashing logic using a new struct and internal function, removing redundant code and adding protocol-based slashing execution. Test helpers were adjusted to simulate time passage using timestamps instead of block numbers. Minor visibility and serialization additions were also made.

Changes

File(s) Change Summary
src/libs/SlashingLib.sol Modified fraud proof window check from block number to block timestamp in internal validation logic.
src/slasher/LinglongSlasher.sol Added SlashingParams struct and _prepareSlashingParams function; refactored slash to use these; removed slashFromOptIn; enhanced protocol-based slashing execution.
test/EigenLayerMiddleware.t.sol, test/SymbioticMiddleware.t.sol Changed test time simulation from incrementing block number by days to incrementing block timestamp by hours.
script/Deployments.s.sol Serialized the deployed Registry contract address under "taiyiAddresses" namespace.
test/utils/EigenlayerDeployer.sol Changed MIN_WITHDRAWAL_DELAY visibility from internal to public.
test/utils/MockChallenger.sol Modified getSlashAmount() to return 1e18 instead of 1 ether for clarity.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant LinglongSlasher
    participant Protocol
    participant Blockchain

    Caller->>LinglongSlasher: slash(delegation, commitment, committer, evidence, challenger)
    LinglongSlasher->>LinglongSlasher: _prepareSlashingParams(...)
    LinglongSlasher->>Protocol: _executeSlashingByProtocol(slashingParams)
    Protocol-->>LinglongSlasher: slashing result
    LinglongSlasher->>LinglongSlasher: mark commitment slashed
    LinglongSlasher->>Caller: emit SlashingResult, return 0
Loading

Suggested reviewers

  • will-luban
  • da-bao-jian

Poem

A hop through time, a tick, a tock,
No longer bound by block on block.
With timestamps now, the fraud-proof’s clear—
The slasher’s logic shifts this year!
In tests and code, the clock prevails,
As rabbits cheer with fluffy tails.
🕰️🐇

Tip

⚡️ Faster reviews with caching
  • CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.

Enjoy the performance boost—your workflow just got faster.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
test/EigenLayerMiddleware.t.sol (1)

988-990: Avoid hard-coded “3 hours” magic number

fraudProofWindow is already stored in the Registry config (7200 s).
Basing the warp on that value will keep the test correct even if the constant changes:

-        // Wait for the fraud proof window to pass
-        vm.warp(block.timestamp + 3 hours);
+        // Wait for fraud-proof window + 1 s to pass
+        vm.warp(block.timestamp + registry.getConfig().fraudProofWindow() + 1);
test/SymbioticMiddleware.t.sol (1)

240-242: Use config value instead of literal “3 hours”

Same reasoning as in the EigenLayer test: couple the warp to the configured window to avoid silent test failures when parameters evolve.

-        // Wait for fraud proof window
-        vm.warp(block.timestamp + 3 hours);
+        // Wait past the fraud-proof window
+        vm.warp(block.timestamp + urcRegistry.getConfig().fraudProofWindow() + 1);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d610ec1 and a023a80.

📒 Files selected for processing (5)
  • lib/urc (1 hunks)
  • src/libs/SlashingLib.sol (1 hunks)
  • src/slasher/LinglongSlasher.sol (1 hunks)
  • test/EigenLayerMiddleware.t.sol (1 hunks)
  • test/SymbioticMiddleware.t.sol (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Foundry Project
🔇 Additional comments (3)
lib/urc (1)

1-1:

Details

❓ Verification inconclusive

Submodule update looks correct; verify for breaking changes.

The lib/urc subproject pointer has been bumped to commit 5f18225c4d027b49084605129b7666a37ea13412. No public interface changes are expected, but please confirm that:

  • The new commit aligns with the intended timestamp-based logic updates.
  • All dependent tests in lib/urc still pass.
  • There are no unintended API changes used by the slashing library.

🏁 Script executed:

#!/bin/bash
# Fetch and diff the submodule changes
git submodule update --init lib/urc
cd lib/urc
git fetch
git diff e0c5c8a8212a04e8844df0268c7de17b9e6b8896 5f18225c4d027b49084605129b7666a37ea13412

# Run the submodule's test suite (if available)
# e.g., cargo test || npm test

Length of output: 70062


Confirm lib/urc submodule API & logic updates

The bump to commit 5f18225… introduces the following core changes in lib/urc:

  • Switched all on-chain timing from block.numberblock.timestamp in Registry.sol.
  • Updated IRegistry & ISlasher interfaces:
    • Timestamp-based fields now use uint48/uint24 widths.
    • Removed the old slashFromOptIn method.
    • Added a committer parameter to the slash signature.
  • Adjusted example contracts (InclusionPreconfSlasher.sol, StateLockSlasher.sol) and scripts to match the new API.
  • Added config/registry.json, expanded scripts folder, and updated Foundry settings.

Please verify that:

  • All downstream code (clients, slashing library, examples) has been updated to call the new slash(..., committer, …) signature and no longer relies on slashFromOptIn.
  • There are no unintended API changes breaking upstream usage.
  • All lib/urc tests still pass (e.g. via forge test or npm test).
  • Integration/slashing tests in the main repo remain green.
src/libs/SlashingLib.sol (1)

308-310:

Details

❓ Verification inconclusive

Possible unit-mismatch in fraud-proof window check

registeredAt is still stored as a block number elsewhere in the library (unregisteredAt is compared with block.number a few lines above).
By switching this clause to block.timestamp you implicitly assume that registeredAt is now a Unix-timestamp. If that field still contains a block number, the comparison mixes blocks with seconds and the fraud-proof window will be orders of magnitude longer than intended.

Action items
• Double-check how OperatorData.registeredAt is written in Registry after this change.
• If it is still a block number, convert it to seconds (or vice-versa) before the addition.
• Consider migrating every *_At field to a single time base to avoid future bugs (the adjacent unregisteredAt clause still uses block.number!).

Example patch if timestamps are desired everywhere:

// Treat all *_At fields as timestamps (seconds)
-        if (unregisteredAt < block.number) {
+        if (unregisteredAt < block.timestamp) {
             revert OperatorUnregistered();
         }

-        if (registeredAt + registry.getConfig().fraudProofWindow > block.timestamp) {
+        if (uint256(registeredAt) + registry.getConfig().fraudProofWindow > block.timestamp) {
             revert OperatorFraudProofPeriodNotOver();
         }

🏁 Script executed:

#!/bin/bash
# Inspect every assignment to `registeredAt` to confirm whether
# a timestamp or block number is stored.
rg --no-heading -n '\.registeredAt\s*='

Length of output: 146


🏁 Script executed:

#!/bin/bash
# Retrieve the OperatorData struct definition to check types of registeredAt and unregisteredAt
rg --no-heading -n 'struct OperatorData' -A10

# Search for assignments to registeredAt across the repository to see if it's set to block.number or block.timestamp
rg --no-heading -n 'registeredAt\s*=' -g '*.sol' -A3

Length of output: 1889


🏁 Script executed:

#!/bin/bash
# Locate the OperatorData struct definition and its field types
rg --no-heading -n 'struct OperatorData' -g 'src/**/*.sol' -A15

# Find the implementation of getOperatorData to see where OperatorData lives
rg --no-heading -n 'function getOperatorData' -g 'src/**/*.sol' -A15

# Search for where operatorData.registeredAt is assigned in registry code
rg --no-heading -n 'operatorData\.registeredAt\s*=' -g 'src/**/*.sol' -A5

Length of output: 212


🏁 Script executed:

#!/bin/bash
# Locate all references to OperatorData to find its struct definition and type of registeredAt
rg --no-heading -n 'OperatorData' -g 'src/**/*.sol'

# Fallback grep in case rg misses it
grep -R -n 'struct OperatorData' src

Length of output: 1114


Verify OperatorData.registeredAt time unit before merging

I wasn’t able to locate the struct OperatorData definition in this repo (it lives in the external IRegistry interface), so please double-check whether its registeredAt field is a block number or a Unix-timestamp. Mixing blocks with seconds here can drastically extend the fraud-proof window if you assume the wrong unit.

Action items
• Confirm the exact type/unit of IRegistry.OperatorData.registeredAt in the registry contract.
• If it’s still stored as a block number, convert it to seconds (or switch the comparison to block.number) before adding fraudProofWindow.
• Consider standardizing all *_At fields (e.g. registeredAt, unregisteredAt, slashedAt) on the same time base to prevent similar bugs in the future.

src/slasher/LinglongSlasher.sol (1)

215-219: Great improvement to function signature readability

Changing from unnamed to named parameters in the slash function significantly improves code readability and maintainability. This change aligns with best practices for interface implementations, making it clearer to developers what each parameter represents while maintaining the same functionality.

da-bao-jian
da-bao-jian previously approved these changes May 7, 2025
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (2)
src/slasher/LinglongSlasher.sol (2)

425-443: ⚠️ Potential issue

Potential zero-ID slashing when operator is not allocated to EigenLayer middleware

operatorSetId defaults to 0. If the loop fails to find a matching set, the code continues with operatorSetId == 0, meaning:

  1. _checkSlashingNotInProgress() runs against set 0 (likely non-existent).
  2. _prepareEigenLayerSlashingParams() builds params for set 0, risking an unintended or failing slash.

Add an explicit check after the loop to guarantee a valid set was found:

for (uint256 i = 0; i < eigenLayerSets.length; i++) {
    if (eigenLayerSets[i].avs == middleware) {
        operatorSetId = eigenLayerSets[i].id;
        break;
    }
}
-// proceeds even if operatorSetId == 0
+if (operatorSetId == 0) {
+    revert OperatorSetNotFound();
+}

486-499: ⚠️ Potential issue

Same missing-check issue for Symbiotic subnetworks

subnetwork is initialised to 0 and used unconditionally when no suitable subnetwork is found, leading to a silent mis-slash or revert inside the Symbiotic middleware.

for (uint256 i = 0; i < subnetworks.length; i++) {
    ...
}
-if (subnetwork == 0) {
-    revert SubnetworkNotFound();
-}

Introduce an explicit revert (or equivalent guard) before continuing.

🧹 Nitpick comments (2)
test/utils/MockChallenger.sol (1)

56-58: Replace the raw literal with a named constant for clarity & future-proofing

1e18 is correct (it equals 1 ether), but the intent—“100 % in WAD units” vs. “slash one ether”—remains implicit. Defining a uint256 internal constant FULL_WAD = 1e18; (or similar) and returning that constant makes the purpose self-evident and avoids “magic numbers”.

+uint256 internal constant FULL_WAD = 1e18; // 100 % in 18-dec WAD units

 function getSlashAmount() external pure returns (uint256) {
-    return 1e18; // 100 WAD percent
+    return FULL_WAD;
 }

Please confirm that downstream middleware interprets the value as a percentage-style WAD and not an absolute token amount; otherwise a full-balance slash might unintentionally become “slash one ether”.

src/slasher/LinglongSlasher.sol (1)

225-232: delegation, committer, and challenger parameters are unused – silences vs. semantics

The new _prepareSlashingParams signature still accepts Delegation calldata delegation, yet the variable is never touched; the same goes for committer and challenger in slash().
Apart from compiler warnings, this is misleading to readers and might hide unfinished logic (e.g., verifying that committer == commitment.slasher). At a minimum mark them _unused to silence warnings; ideally remove or implement the intended checks.

-function _prepareSlashingParams(
-    Delegation calldata delegation,
+function _prepareSlashingParams(
+    Delegation calldata /* delegation */,

Similar change inside slash():

-    address committer,
+    address /* committer */,
 ...
-    address challenger
+    address /* challenger */
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between b7c9e0e and ca232ad.

📒 Files selected for processing (5)
  • script/Deployments.s.sol (1 hunks)
  • src/slasher/LinglongSlasher.sol (2 hunks)
  • test/EigenLayerMiddleware.t.sol (2 hunks)
  • test/utils/EigenlayerDeployer.sol (1 hunks)
  • test/utils/MockChallenger.sol (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • script/Deployments.s.sol
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/EigenLayerMiddleware.t.sol
🔇 Additional comments (1)
test/utils/EigenlayerDeployer.sol (1)

111-111: Making MIN_WITHDRAWAL_DELAY public improves test accessibility

Changing the visibility modifier from internal to public is appropriate here. This allows external contracts and test code to directly access this constant value, which facilitates more precise control and readability in test scenarios involving withdrawal timing logic.

@will-luban will-luban requested a review from da-bao-jian May 8, 2025 04:11
@will-luban will-luban mentioned this pull request May 8, 2025
@da-bao-jian da-bao-jian merged commit 1f41797 into main May 8, 2025
2 checks passed
@da-bao-jian da-bao-jian deleted the update-urc branch May 8, 2025 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants